Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dns: default to verbatim=true in dns.lookup() #39987

Closed
wants to merge 18 commits into from

Conversation

treysis
Copy link
Contributor

@treysis treysis commented Sep 3, 2021

Switch the default from false (reorder the result so that IPv4 addresses
come before IPv6 addresses) to true (return them exactly as the resolver
sent them to us.)

After having the option to manually override the default behavior, this is another attempt at changing the default behavior.

Fixes: #31566
Refs: #6307
Refs: #20710
Refs: #38099
Reissue of #31567
Reissue of #37681
Reissue of #37931

@nodejs-github-bot nodejs-github-bot added dns Issues and PRs related to the dns subsystem. needs-ci PRs that need a full CI run. labels Sep 3, 2021
@treysis treysis force-pushed the verbatim-true-v17 branch 4 times, most recently from 879d1ff to e37e2be Compare September 3, 2021 20:40
bnoordhuis and others added 2 commits September 3, 2021 22:43
Switch the default from false (reorder the result so that IPv4 addresses
come before IPv6 addresses) to true (return them exactly as the resolver
sent them to us.)

Fixes: nodejs#31566
Refs: nodejs#6307
Refs: nodejs#20710
Refs: nodejs#38099
Reissue of nodejs#31567
Reissue of nodejs#37681
Reissue of nodejs#37931
lib/internal/dns/utils.js Outdated Show resolved Hide resolved
@aduh95 aduh95 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 3, 2021
@treysis
Copy link
Contributor Author

treysis commented Sep 3, 2021

Now again we have some failing tests. Do you think I should remove where appropriate to listen on 127.0.0.1 an just listen "generally"? Or force listening on 127.0.0.1 and force connect on 127.0.0.1?

@treysis
Copy link
Contributor Author

treysis commented Sep 4, 2021

Oooh, my careless mistakes. Let's hope it turns green after this run so we can spin up a CI job.

@treysis
Copy link
Contributor Author

treysis commented Sep 4, 2021

Alright, can we get a CI @aduh95 ?

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2021
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/39778/

@treysis
Copy link
Contributor Author

treysis commented Sep 5, 2021

Can we get a CI already before passing the other checks? Because I just need to see why some stuff is failing on CI @aduh95
I'll fix the linter errors before, though.

fpm-peter pushed a commit to fpm-git/apm-agent-nodejs that referenced this pull request Aug 20, 2024
…7.0.0.1' rather than 'localhost' (elastic#3049)

Starting in node v17 the defaults for DNS resolution order was
changed (nodejs/node#39987) such that
`dns.lookup()` no longer sorted IPv4 addresses first. This impacts
usage of the *default* APM Server URL (the `serverUrl` config var),
'http://localhost:8200', when using node >=17 because the APM server
only binds to the IPv4 address by default
(elastic/apm-server#1405).

Fixes: elastic#3045
Refs: elastic/apm#727
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dns Issues and PRs related to the dns subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. tsc-agenda Issues and PRs to discuss during the meetings of the TSC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dns: default to verbatim=true in dns.lookup()
9 participants